https: refactor to use http internals #16395

bengl · 2017-10-23T00:28:43Z

Rather than using http, use _http_client, etc. directly.

Also moving all the exports to the bottom, in line with most of the rest
of the codebase.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

https

bengl · 2017-10-23T00:33:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10912/

bengl · 2017-10-23T06:57:54Z

/cc @nodejs/http

apapirovski

👍 LGTM. One tiny (totally optional) nit.

apapirovski · 2017-10-23T12:05:09Z

lib/https.js

 const util = require('util');
+const agent = require('_http_agent');
+const server = require('_http_server');


Just a nit but could these be:

const { Agent: HttpAgent } = require('_http_agent'); const { Server: HttpServer, _connectionListener: httpConnectionListener } = require('_http_server');

The usage further down feels a bit clearer that way Server.prototype.setTimeout = HttpServer.prototype.setTimeout;

cjihrig

Code LGTM, but might need to consider semverness if monkey patching code could be impacted.

cjihrig · 2017-10-23T12:48:17Z

lib/https.js

@@ -237,11 +235,20 @@ exports.request = function request(options, cb) {
    options = util._extend({}, options);
  }
  options._defaultAgent = globalAgent;
-  return http.request(options, cb);
-};
+  return new ClientRequest(options, cb);


Could this PR break existing code that relies on monkey patching?

Yeah, definitely a good concern. I'm not immediately aware of modules monkeypatching the client side code but it is definitely something we need to look for before landing this.

If anyone is monkey patching http.request, then http.ClientRequest and _http_client.ClientRequest also need to be monkey patched since they're all exposed and equivalent. I've seen userland code using all three.

That https.request currently calls http.request is an implementation detail (i.e. semver-patch). Users should not be relying on that.

Just found this commit after noticing our tests for https break. Turns out our instrumentation relied on this implementation detail. :)

It seems to be breaking GCP's Stackdriver Trace as well.

revert for v8.x in #16660

Rather than using `http`, use `_http_client`, etc. directly. Also moving all the exports to the bottom, in line with most of the rest of the codebase.

bengl · 2017-10-25T04:29:52Z

@apapirovski Your nit is now addressed.

CI: https://ci.nodejs.org/job/node-test-pull-request/10942/

jasnell · 2017-10-25T04:32:59Z

I'd like to see a CITGM run and some verification that express and hapi still work with this.

bengl · 2017-10-25T04:50:38Z

@jasnell Here's a CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1027/

Also I just ran npm it on express and hapi with this patch, all seemed good.

jasnell · 2017-10-25T04:52:33Z

Awesome. Thanks!

Rather than using `http`, use `_http_client`, etc. directly. Also moving all the exports to the bottom, in line with most of the rest of the codebase. PR-URL: #16395 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

bengl · 2017-10-26T22:48:54Z

Landed in 5118f31

Rather than using `http`, use `_http_client`, etc. directly. Also moving all the exports to the bottom, in line with most of the rest of the codebase. PR-URL: #16395 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This reverts commit 5118f31. It is breaking code in the wild that depends on the original behavior to do tracing. I don't think we need to necessarily fix this in 8.x but we might want to reclassify the original commit as Semver Major Refs: nodejs#16395

Rather than using `http`, use `_http_client`, etc. directly. Also moving all the exports to the bottom, in line with most of the rest of the codebase. PR-URL: nodejs/node#16395 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This reverts commit 5118f31. It is breaking code in the wild that depends on the original behavior to do tracing. I don't think we need to necessarily fix this in 8.x but we might want to reclassify the original commit as Semver Major PR-URL: #16660 Refs: #16395 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

Rather than using `http`, use `_http_client`, etc. directly. Also moving all the exports to the bottom, in line with most of the rest of the codebase. PR-URL: nodejs/node#16395 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the https Issues or PRs related to the https subsystem. label Oct 23, 2017

apapirovski approved these changes Oct 23, 2017

View reviewed changes

cjihrig approved these changes Oct 23, 2017

View reviewed changes

lpinca approved these changes Oct 24, 2017

View reviewed changes

https: refactor to use http internals

254aa3f

Rather than using `http`, use `_http_client`, etc. directly. Also moving all the exports to the bottom, in line with most of the rest of the codebase.

bengl force-pushed the bengl/refactorhttps branch from a3cedb1 to 254aa3f Compare October 25, 2017 04:26

jasnell approved these changes Oct 25, 2017

View reviewed changes

bengl closed this Oct 26, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

kjin mentioned this pull request Nov 1, 2017

https.request doesn't work in Node 8.9+ googleapis/cloud-trace-nodejs#589

Closed

MylesBorins mentioned this pull request Nov 1, 2017

[v8.x] https: revert "refactor to use http internals" #16660

Closed

MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels Nov 1, 2017

MylesBorins added dont-land-on-v6.x labels Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

https: refactor to use http internals #16395

https: refactor to use http internals #16395

bengl commented Oct 23, 2017

bengl commented Oct 23, 2017

bengl commented Oct 23, 2017

apapirovski left a comment

apapirovski Oct 23, 2017 •

edited

Loading

cjihrig left a comment

cjihrig Oct 23, 2017

jasnell Oct 23, 2017

bengl Oct 24, 2017

NatalieWolfe Oct 31, 2017

ofrobots Nov 1, 2017

MylesBorins Nov 1, 2017

bengl commented Oct 25, 2017

jasnell commented Oct 25, 2017

bengl commented Oct 25, 2017

jasnell commented Oct 25, 2017

bengl commented Oct 26, 2017

https: refactor to use http internals #16395

https: refactor to use http internals #16395

Conversation

bengl commented Oct 23, 2017

Checklist

Affected core subsystem(s)

bengl commented Oct 23, 2017

bengl commented Oct 23, 2017

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig Oct 23, 2017

Choose a reason for hiding this comment

jasnell Oct 23, 2017

Choose a reason for hiding this comment

bengl Oct 24, 2017

Choose a reason for hiding this comment

NatalieWolfe Oct 31, 2017

Choose a reason for hiding this comment

ofrobots Nov 1, 2017

Choose a reason for hiding this comment

MylesBorins Nov 1, 2017

Choose a reason for hiding this comment

bengl commented Oct 25, 2017

jasnell commented Oct 25, 2017

bengl commented Oct 25, 2017

jasnell commented Oct 25, 2017

bengl commented Oct 26, 2017

apapirovski Oct 23, 2017 •

edited

Loading